Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update posterior predictive sampling and notebook #5268

Merged
merged 9 commits into from
Dec 20, 2021
Merged

Conversation

OriolAbril
Copy link
Member

  • some updates to posterior predictive sampling and conversion
  • update posterior predictive notebook

notebook was executed on this branch, not on the released beta though

Thank your for opening a PR!

Depending on what your PR does, here are a few things you might want to address in the description:

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@fonnesbeck
Copy link
Member

What is the (posterior_predictive)= header for? Is it still needed?

@OriolAbril
Copy link
Member Author

What is the (posterior_predictive)= header for? Is it still needed?

This is needed to add a target pointing to the title so we can then reference the notebook with sphinx cross-references.
It is always better to use them instead of using urls because then links are more robust and will work even if we move the
notebook to a different location for example, however, now that we have versioned docs we can't use urls at all anywhere. Otherwise you are browsing 4.1 docs, click on a link and without warning start browsing 3.11 docs or 4.3

@codecov
Copy link

codecov bot commented Dec 18, 2021

Codecov Report

Merging #5268 (676808f) into main (cce1613) will decrease coverage by 0.13%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5268      +/-   ##
==========================================
- Coverage   79.17%   79.04%   -0.14%     
==========================================
  Files          88       87       -1     
  Lines       14346    14334      -12     
==========================================
- Hits        11359    11330      -29     
- Misses       2987     3004      +17     
Impacted Files Coverage Δ
pymc/plots/__init__.py 95.65% <ø> (-0.19%) ⬇️
pymc/sampling.py 86.23% <63.15%> (-0.57%) ⬇️
pymc/backends/arviz.py 89.12% <93.75%> (-0.40%) ⬇️
pymc/util.py 74.83% <100.00%> (-0.17%) ⬇️
pymc/backends/ndarray.py 79.64% <0.00%> (-9.74%) ⬇️
pymc/model.py 83.26% <0.00%> (ø)
pymc/distributions/transforms.py 92.55% <0.00%> (+0.59%) ⬆️

pymc/sampling.py Show resolved Hide resolved
pymc/sampling.py Outdated Show resolved Hide resolved
OriolAbril and others added 2 commits December 19, 2021 20:35
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
@michaelosthege michaelosthege merged commit cf95a78 into main Dec 20, 2021
@michaelosthege michaelosthege deleted the pp_notebook branch December 20, 2021 10:12
k,
)
warning_vars.append(k)
warnings.warn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OriolAbril I think you intended for this warning to only be printed if warnings_vars was not empty. Is this correct? I am currently always getting the output:

/Users/thomas/repo/pymc/pymc/backends/arviz.py:389: UserWarning: The shape of variables  in posterior_predictive group is not compatible with number of chains and draws. The automatic dimension naming might not have worked. This can also mean that some draws or even whole chains are not represented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be extra clear: The warning prints a space in place of {', '.join(warning_vars)}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry I also missed it in my review.
It needs to be indented into an if warning_vars.
Thanks for reporting this! Do you want to open a PR? Otherwise I'll do it in the morning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return prior & posterior predictive samples as xarray object Set keep_size=True in v4
5 participants